New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

deps: update tempfile to 3 #900

Merged

bors merged 3 commits into nix-rust:master from ignatenkobrain:master

Sep 2, 2018

Contributor

ignatenkobrain commented May 11, 2018

Signed-off-by: Igor Gnatenko [email protected]

Contributor Author

ignatenkobrain commented May 11, 2018

I'm not sure if you want to have prefixed directories, but kept it as it was.

Contributor Author

ignatenkobrain commented May 11, 2018

(except for the doctest for simplicity sake)

asomers reviewed

View reviewed changes

Member

asomers left a comment

Overall, it looks good. But I think we should be consistent about using either anonymous or named temporary files/directories. The preference should probably be anonymous tempfile > named tempfile > anonymous tempdir > named tempdir.

test/sys/test_socket.rs Outdated


		let tempdir = TempDir::new("test_getsockname").unwrap();
		let tempdir = tempfile::Builder::new()

Member

asomers May 11, 2018

Shouldn't this test work even with an anonymous tempdir, like this? I think anonymous tempdirs still have names; only anonymous tempfiles don't.

let tempdir = TempDir::new();
let sockname = tempdir.path().join("sock")

Contributor Author

ignatenkobrain May 11, 2018

it should, just wanted to keep prefixes as it was in original code. If this is not needed, I'm going to drop it.

test/sys/test_uio.rs

@@ @@ -114,7 +113,10 @@ fn test_pwrite() { @@
               fn test_pread() {
                   use std::io::Write;
-                  let tempdir = TempDir::new("nix-test_pread").unwrap();

Member

asomers May 11, 2018

In this case, I think you can use an anonymous temp file like this. Less code, and no Rust cleanup required. test_pwrite already does just this.

let mut file = tempfile().unwrap();

Contributor

Susurrus commented Jun 2, 2018

@ignatenkobrain friendly ping

Contributor Author

ignatenkobrain commented Jun 22, 2018

sorry for delay, was busy with $dayjob. Updating now.

ignatenkobrain force-pushed the master branch from b3ca1a7 to 6d9e090 Compare

July 4, 2018 12:52

Contributor Author

ignatenkobrain commented Jul 4, 2018

rebased + updated.

Member

asomers commented Jul 4, 2018

Looks like you have a basic syntax error. Did you test locally before pushing?

ignatenkobrain force-pushed the master branch from 6d9e090 to bfdb0f8 Compare

July 4, 2018 15:45

Contributor Author

ignatenkobrain commented Jul 7, 2018

@asomers passes now

asomers approved these changes

View reviewed changes

Member

asomers left a comment

LGTM. @Susurrus any feelings?

Contributor

Susurrus commented Jul 11, 2018

I'd agree with consistently using anonymous directories. I'd also suggest we be consistent with using .unwrap() versus .unwrap_or_else() on the Result returned by tempdir(). It's an unrecoverable error, but having some more details is nice, so I'd suggest using .expect(...) here.

Member

asomers commented Jul 15, 2018

@Susurrus does that mean you don't approve? If so, please give a negative review to indicate that the PR still needs some work.

Susurrus suggested changes

View reviewed changes

Contributor

Susurrus left a comment

We should have consistent usages of tempdir(), and I don't think we need to prefix or do any real error checking on this. If temp directories are failing, we likely have bigger issues here.

test/test_fcntl.rs Outdated

@@ @@ -30,7 +29,9 @@ fn test_openat() { @@
               #[test]
               fn test_readlink() {
-                  let tempdir = TempDir::new("nix-test_readdir")
+                  let tempdir = tempfile::Builder::new()

Contributor

Susurrus Jul 15, 2018

Just tempfile::tempdir().unwrap(); should be fine.

test/test_mount.rs Outdated

@@ @@ -32,7 +32,9 @@ exit 23"; @@
                   const NONE: Option<&'static [u8]> = None;
                   pub fn test_mount_tmpfs_without_flags_allows_rwx() {
-                      let tempdir = TempDir::new("nix-test_mount")
+                      let tempdir = tempfile::Builder::new()

Contributor

Susurrus Jul 15, 2018

Just tempfile::tempdir().unwrap(); should be fine.

test/test_mount.rs Outdated

@@ @@ -89,7 +91,9 @@ exit 23"; @@
                   }
                   pub fn test_mount_rdonly_disallows_write() {
-                      let tempdir = TempDir::new("nix-test_mount")
+                      let tempdir = tempfile::Builder::new()

Contributor

Susurrus Jul 15, 2018

Just tempfile::tempdir().unwrap(); should be fine.

test/test_mount.rs Outdated

@@ @@ -107,7 +111,9 @@ exit 23"; @@
                   }
                   pub fn test_mount_noexec_disallows_exec() {
-                      let tempdir = TempDir::new("nix-test_mount")
+                      let tempdir = tempfile::Builder::new()

Contributor

Susurrus Jul 15, 2018

Just tempfile::tempdir().unwrap(); should be fine.

test/test_mount.rs Outdated

@@ @@ -146,12 +152,16 @@ exit 23"; @@
                   }
                   pub fn test_mount_bind() {
-                      let tempdir = TempDir::new("nix-test_mount")
+                      let tempdir = tempfile::Builder::new()

Contributor

Susurrus Jul 15, 2018

Just tempfile::tempdir().unwrap(); should be fine.

test/test_stat.rs Outdated

@@ @@ -106,7 +115,10 @@ fn test_stat_fstat_lstat() { @@
               #[test]
               fn test_fchmod() {
-                  let tempdir = TempDir::new("nix-test_fchmod").unwrap();
+                  let tempdir = tempfile::Builder::new()

Contributor

Susurrus Jul 15, 2018

Just tempfile::tempdir().unwrap(); should be fine.

test/test_stat.rs Outdated

@@ @@ -128,7 +140,10 @@ fn test_fchmod() { @@
               #[test]
               fn test_fchmodat() {
-                  let tempdir = TempDir::new("nix-test_fchmodat").unwrap();
+                  let tempdir = tempfile::Builder::new()

Contributor

Susurrus Jul 15, 2018

Just tempfile::tempdir().unwrap(); should be fine.

test/test_unistd.rs Outdated

@@ @@ -84,7 +81,10 @@ fn test_mkstemp_directory() { @@
               #[test]
               fn test_mkfifo() {
-                  let tempdir = TempDir::new("nix-test_mkfifo").unwrap();
+                  let tempdir = tempfile::Builder::new()

Contributor

Susurrus Jul 15, 2018

Just tempfile::tempdir().unwrap(); should be fine.

test/test_unistd.rs Outdated

@@ @@ -286,7 +286,10 @@ fn test_fchdir() { @@
                   #[allow(unused_variables)]
                   let m = ::CWD_MTX.lock().expect("Mutex got poisoned by another test");
-                  let tmpdir = TempDir::new("test_fchdir").unwrap();
+                  let tmpdir = tempfile::Builder::new()

Contributor

Susurrus Jul 15, 2018

Just tempfile::tempdir().unwrap(); should be fine.

test/test_unistd.rs Outdated

@@ @@ -302,7 +305,10 @@ fn test_getcwd() { @@
                   #[allow(unused_variables)]
                   let m = ::CWD_MTX.lock().expect("Mutex got poisoned by another test");
-                  let tmpdir = TempDir::new("test_getcwd").unwrap();
+                  let tmpdir = tempfile::Builder::new()

Contributor

Susurrus Jul 15, 2018

Just tempfile::tempdir().unwrap(); should be fine.

ignatenkobrain force-pushed the master branch 2 times, most recently from fd85572 to 9e1caa4 Compare

July 17, 2018 07:18

Contributor Author

ignatenkobrain commented Jul 17, 2018

Updated to address all points from @Susurrus

Contributor

Susurrus commented Jul 24, 2018

Test failures on i386 freebsd 11. Travis reported a spurious failure I'm rerunning.

Susurrus suggested changes

View reviewed changes

test/sys/test_socket.rs Outdated

@@ @@ -90,9 +90,9 @@ pub fn test_abstract_uds_addr() { @@
               pub fn test_getsockname() {
                   use nix::sys::socket::{socket, AddressFamily, SockType, SockFlag};
                   use nix::sys::socket::{bind, SockAddr};
-                  use tempdir::TempDir;
+                  use tempfile;

Contributor

Susurrus Jul 24, 2018

This isn't necessary, correct?

Contributor

bachp Aug 9, 2018

@Susurrus I don't understand what you mean?

Contributor

Susurrus Aug 10, 2018

Sorry, don't know what I was talking about here. But since none of these are conditionally compiled I'd suggest we put a single use statement at the top of the file and get rid of all of the additional ones.

Contributor

bachp commented Aug 8, 2018

It seems like tempfile requires rand 0.5 which in turn requires Rust 1.22 or higher:

The 0.5 release of Rand requires Rustc version 1.22 or greater. Rand 0.4 and 0.3 (since approx. June 2017) require Rustc version 1.15 or greater. Subsets of the Rand code may work with older Rust versions, but this is not supported.

Is it possible to bump the required rust version for nix to 1.22 or higher?

I'm also interested in having this in for #930

Member

asomers commented Aug 9, 2018

There's nothing special about 1.20.0. You can bump the minimum compiler version if it's for a good reason.

Contributor

bachp commented Aug 9, 2018

@asomers I already tried this in #930 but I don't know how to bump the version for the freebsd buildbot? Does this need to be done by somebody?

Member

asomers commented Aug 10, 2018

Correct. I have to update the buildbots outside of Github. I'll do that after you settle on a Rustc version. Will it be 1.22.0 or 1.24.1?

Susurrus approved these changes

View reviewed changes

Contributor

Susurrus commented Aug 11, 2018

These changes all look good to me. Just needs the version stuff sorted out. I will say that I'd like to bump the version as little as possible. If 1.22.0 works, I'd prefer we use that.

Contributor

Susurrus commented Aug 15, 2018

I believe the buildbots have been updated to 1.22.1, @ignatenkobrain will you have time to work on this in the near future? We'd like to get this merged before some other PRs.

Susurrus mentioned this pull request

Add wrapper for linux kernel module loading #930

Merged

ignatenkobrain force-pushed the master branch 2 times, most recently from b8e8454 to 6ccfc37 Compare

August 15, 2018 05:23

Contributor Author

ignatenkobrain commented Aug 15, 2018

we should be all good.

bachp suggested changes

View reviewed changes

Contributor

bachp left a comment

As a general remark, I would try to use tempdir() and tempfile() functions wherever possible. They should be imported with use tempfile::{tempdir, tempfile} (whichever is required). This would make the whole thing more consistent and there is no need to import self. The only exception is the NamedTempFile that can't be replaced by tempfile().

src/unistd.rs Outdated

               ///
               /// fn main() {
-              ///     let tmp_dir1 = TempDir::new("test_mkdir").unwrap();
+              ///     let tmp_dir1 = TempDir::new().unwrap();

Contributor

bachp Aug 15, 2018

use tempfile::tempdir().unwrap() here

src/unistd.rs Outdated

               /// extern crate nix;
               ///
               /// use nix::unistd;
               /// use nix::sys::stat;
-              /// use tempdir::TempDir;
+              /// use tempfile::TempDir;

Contributor

bachp Aug 15, 2018

only use tempfile or tempfile::tempdir so it can be uses as tempdir().unwrap()

src/unistd.rs Outdated

               ///
               /// fn main() {
-              ///     let tmp_dir = TempDir::new("test_fifo").unwrap();
+              ///     let tmp_dir = TempDir::new().unwrap();

Contributor

bachp Aug 15, 2018

use tempfile::tempdir().unwrap() here, same as above

test/sys/test_uio.rs Outdated

@@ @@ -5,8 +5,7 @@ use std::{cmp, iter}; @@
               use std::fs::{OpenOptions};
               use std::os::unix::io::AsRawFd;
-              use tempdir::TempDir;
-              use tempfile::tempfile;
+              use tempfile::{self, tempfile};

Contributor

bachp Aug 15, 2018

use tempfile::{tempdir, tempfile} no need to import self

test/sys/test_uio.rs Outdated

@@ @@ -114,7 +113,7 @@ fn test_pwrite() { @@
               fn test_pread() {
                   use std::io::Write;
-                  let tempdir = TempDir::new("nix-test_pread").unwrap();
+                  let tempdir = tempfile::tempdir().unwrap();

Contributor

bachp Aug 15, 2018

With use tempfile::{tempdir, tempfile} this becomes just tempdir()

ignatenkobrain added 2 commits

August 15, 2018 11:35


          travis: bump rustc to 1.22.1

102f5c4

Signed-off-by: Igor Gnatenko <[email protected]>


          sendmsg: remove unneeded mut

32fece2

Signed-off-by: Igor Gnatenko <[email protected]>

ignatenkobrain force-pushed the master branch from 1550373 to 19e420f Compare

August 15, 2018 09:35

Contributor Author

ignatenkobrain commented Aug 15, 2018

@bachp fixed.

Contributor

Susurrus commented Aug 15, 2018

Looks good, but needs a CHANGELOG entry under the CHANGED entry with "Increased required Rust version to 1.22.1". Should also update the required version listed in the README. Please do these changes in the last commit. Then LGTM. @asomers anything on your end?

Member

asomers commented Aug 15, 2018

No, I agree with you, @Susurrus . Just needs a CHANGELOG entry.

Contributor

Susurrus commented Aug 17, 2018

@ignatenkobrain Would you be able to update the CHANGELOG and README and squash that into your last commit? This is otherwise ready to merge.

asomers mentioned this pull request

new dir module #916

Merged

Contributor

Susurrus commented Sep 1, 2018

Pinging you on this @ignatenkobrain.

Contributor Author

ignatenkobrain commented Sep 1, 2018

on it.


          deps: update tempfile to 3

ecad72a

Signed-off-by: Igor Gnatenko <[email protected]>

ignatenkobrain force-pushed the master branch from 19e420f to ecad72a Compare

September 1, 2018 09:39

Contributor Author

ignatenkobrain commented Sep 1, 2018

done

Contributor

Susurrus commented Sep 2, 2018

bors r+

bors bot added a commit that referenced this pull request


          Merge #900

82b51a2

900: deps: update tempfile to 3 r=Susurrus a=ignatenkobrain

Signed-off-by: Igor Gnatenko <[email protected]>

Co-authored-by: Igor Gnatenko <[email protected]>

Contributor

bors bot commented Sep 2, 2018

Build succeeded

bors bot merged commit ecad72a into nix-rust:master

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet